-
Notifications
You must be signed in to change notification settings - Fork 97
fix: remove custom proxy handling #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Undici has added native support for proxy handling, so it is no longer necessary for us to implement our own custom proxy handling.
47d08ea
to
98eac26
Compare
👋 @dmitrijrub @mikakesan Since you helped us test proxy functionality (#99) when we added support for it in #102, would you be willing to help us test this branch ( |
hi @parkerbxyz, what are the changes? should I just use |
Yes, exactly! 🙂 |
hmmm, looks like its failing
|
Noting this in case it is helpful later: nodejs/undici#1650 (comment) |
`EnvHttpProxyAgent` automatically reads the proxy configuration from the environment variables.
bbbe985
to
1f00388
Compare
Proxy support should be automatic
Thank you so much for testing that, @dmitrijrub! I've made some changes in hopes of fixing the failure you encountered. Would you mind testing again and letting us know how it goes, please? 🙏 |
Tests are currently failing due to our current use of the global dispatcher in our tests and local dispatcher in the fetch method. The problem can be reproduced with the following example: import { EnvHttpProxyAgent, MockAgent, setGlobalDispatcher } from "undici";
const mockAgent = new MockAgent();
mockAgent.disableNetConnect();
setGlobalDispatcher(mockAgent);
const mockPool = mockAgent.get("https://example.test");
mockPool
.intercept({
path: `/`,
method: "GET",
})
.reply(200);
await fetch("https://example.test", {
// Comment out the following line to see the test succeed
dispatcher: new EnvHttpProxyAgent(),
});
console.log("OK"); The global dispatcher is not being used when a local dispatcher is set. We need to verify if this is a feature or a bug. |
|
Deno appears to have native proxy support: https://docs.deno.com/runtime/reference/env_variables/#special-environment-variables |
I'm down for using Deno instead of Node.js, as long as we still generate the built files with all dependencies and execute that code instead of loading dependencies dynamically at run time. |
I just received a really helpful PR here: peter-evans/create-pull-request#3475 And that lead me to realise that proxy support can be simplified to this: peter-evans/create-pull-request#3483 It passes the proxy support tests I have, so looks good. Just thought I would mention this in case it helps here. |
Thank you @peter-evans! It looks like you use We are interested in your test setup though and couldn't quite figure it out. Can you give us some pointers how you test the env-based proxy handling? |
I see. Well I'm interested to see what solution you end up with here then.
I created a small docker image to launch a forward proxy based on glider. In the test, I start the forward proxy and then configure the firewall on the actions runner. The firewall settings deny outgoing, except through the proxy. So it makes sure that the action is definitely using the proxy when it's configured. |
It looks like Node.js v24 is going to include proxy support: |
The undici package has been removed from the dependencies in package.json and package-lock.json, likely because it is no longer needed in the project.
🎉 This PR is included in version 3.0.0-beta.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Unfortunately, even with Node 24, it's not working out of the box. Now We see two ways to support the proxy behavior
import { spawn } from "child_process";
run();
async function run() {
if (
!process.env.NODE_USE_ENV_PROXY &&
(process.env.HTTP_PROXY || process.env.HTTPS_PROXY)
) {
// spawn itself with NODE_USE_ENV_PROXY=1
const child = spawn(process.execPath, process.argv.slice(1), {
env: { ...process.env, NODE_USE_ENV_PROXY: "1" },
stdio: "inherit",
});
child.on("exit", (code) => process.exit(code));
return;
}
const response = await fetch("https://example.com");
console.log(await response.text());
} |
Undici has added native support for proxy handling, so it is no longer necessary for us to have our own custom proxy handling.
Reverts #102 and resolves #134.
Depends on nodejs/node#43187.
Resources